inet_ntop and inet_pton are available on Vista+.

We are dropping XP support from this 53. When trying to bump WINVER to 0x0601, we cannot built nrappkit. inet_ptoa etc are supported from Vista+, so we should use OS API when WINVER >= 0x600. Also, our toolchain support is VS2015+. So snprintf impl is unnecessary.

Note that nrappkit is a third-party import, which may affect what we want to remove (i.e. does nrappkit want to drop XP support?)

Comment on attachment 8817481 [details] [diff] [review] Cannot compile nrappkit with WINVER=0x0600 or later Review of attachment 8817481 [details] [diff] [review]: ----------------------------------------------------------------- As Randel pointed out nrappkit is an external library I think we should not delete the code supporting Win XP. Could please submit a patch which just disables all the functions supported by >= Win Vista?

I would also be interested in seeing the actual compiler error you get :-)

:drno, As generally, WINVER defines minimal windows version. So if WINVER is 0x0601, minimal requirement of compiled binary is Windows 7+. Actually, we use 0x501 in old-configure.in for XP support. So even if I apply this fix, requirement is still XP+ with WINVER=0x0501. This issue is a regression by bug 797262 (reviewed by :drno). Although :bwc adds IPv6 supports for nrappkit, why don't you and :bwc add patch file against upstream of 3rd party library? :ekr, where the latest repository of nrappkit (http://nrappkit.sourceforge.net/)? Is this still updated?

:m_kato, nrappkit isn't being well-maintained. For now, I think this is a good change.

I don't see how bug 797262 has caused a regression here, as that bug only added inet_pton, but inet_ntop existed in nrappkit already before that patch landed. But besides that nit-picking... Wouldn't it make sense to either: A) Guard the snprintf() implementation with if-def's for the tool chain (so the VS version) and the inet_pton and inet_ntop implementations with if-def's for the Windows version. B) Or since we no longer want to build on Win XP and our min tool chain is recent enough remove all three functions: snprintf, inet_ntop and inet_pton.

(In reply to Nils Ohlmeier [:drno] from comment #7) > I don't see how bug 797262 has caused a regression here, as that bug only > added inet_pton, but inet_ntop existed in nrappkit already before that patch > landed. But besides that nit-picking... This is simple. In SDK header (WS2tcpip.h) #if (NTDDI_VERSION >= NTDDI_VISTA) WINSOCK_API_LINKAGE INT WSAAPI inet_pton( ... So when WINVER define is 0x0600 or higher, inet_pton is duplicated define. So it causes compiling error. This defines are from bug 797262. > Wouldn't it make sense to either: > A) Guard the snprintf() implementation with if-def's for the tool chain (so > the VS version) and the inet_pton and inet_ntop implementations with > if-def's for the Windows version. At first, our minimal requirement is VS2015 update 3+. So we don't support VS2013. It means that snprintf implementation is unnecessary. Also, from Vista, inet_pton and inet_ntop are implemented by winsock. > B) Or since we no longer want to build on Win XP and our min tool chain is > recent enough remove all three functions: snprintf, inet_ntop and inet_pton. Our installer drops XP support by bug 1305453. If WebRTC team keeps XP compatible on source code, I will create new fix that adds WINVER 0x0501 at force into nrappkit.

(In reply to Makoto Kato [:m_kato] from comment #8) > Our installer drops XP support by bug 1305453. If WebRTC team keeps XP > compatible on source code, I will create new fix that adds WINVER 0x0501 at > force into nrappkit. Since utils.h is used from many files, these already includes winsock headers such as WS2tcpip.h. So this cannot resolved by "#undef WINVER" & "#define WINVER 0x0501" hacks.

(In reply to Makoto Kato [:m_kato] from comment #8) > (In reply to Nils Ohlmeier [:drno] from comment #7) > > I don't see how bug 797262 has caused a regression here, as that bug only > > added inet_pton, but inet_ntop existed in nrappkit already before that patch > > landed. But besides that nit-picking... > > This is simple. In SDK header (WS2tcpip.h) > > #if (NTDDI_VERSION >= NTDDI_VISTA) > WINSOCK_API_LINKAGE > INT > WSAAPI > inet_pton( > ... > > So when WINVER define is 0x0600 or higher, inet_pton is duplicated define. > So it causes compiling error. This defines are from bug 797262. Yes inet_pton got added though bug 797262. But inet_ntop, which causes the same compiler errors, got added via bug 825927 back in 2013. > > B) Or since we no longer want to build on Win XP and our min tool chain is > > recent enough remove all three functions: snprintf, inet_ntop and inet_pton. > > Our installer drops XP support by bug 1305453. If WebRTC team keeps XP > compatible on source code, I will create new fix that adds WINVER 0x0501 at > force into nrappkit. Sorry I don't get why you reply onto my question/proposal to remove all three functions with "WebRTC ream keeps XP compatible on source code". To me that is the opposite of what I asked/proposed. Since I never got any links to compiler logs I spend my day yesterday building several times on my win laptop. And to answer my own question: we can't remove the inet_ntop and inet_pton helper functions from nrappkit, because we are still building for XP right now. And removing them would break the builds. So in summary we should probably land your original patch (which I'm going to r+ now) and then file a follow-up bug to remove the remaining XP code once we no longer build on XP.

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/mozilla-inbound/rev/86a93a4e4231 Cannot compile nrappkit with WINVER=0x0600 or later. r=drno